Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add EmbeddedModelField #151

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

timgraham
Copy link
Collaborator

@timgraham timgraham commented Oct 4, 2024

Rudimentary forms support.
Limited schema change support (no changing of embedded models).
Limited querying support (no field validation (see timgraham#3)
No aggregation support (though much seems to work automagically).

fixes #103

@timgraham timgraham force-pushed the embedded-model-field branch from bec8461 to a6479ba Compare October 4, 2024 15:13
@ShaneHarvey
Copy link

Just want to make sure if we're copying code that we attribute it properly according to the django-nonrel license.

@timgraham
Copy link
Collaborator Author

Certainly. THIRD-PARTY-NOTICES could be amended.

Copy link
Collaborator

@Jibola Jibola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, LGTM! Before merge, I'd love to go over the commented pieces again, but thank you for transferring the tests. I think they covered all the cases we'd be curious about as well.

django_mongodb/fields/embedded_model.py Outdated Show resolved Hide resolved
django_mongodb/fields/embedded_model.py Outdated Show resolved Hide resolved
tests/mongo_fields/models.py Outdated Show resolved Hide resolved
tests/mongo_fields/test_embedded_model.py Outdated Show resolved Hide resolved
tests/mongo_fields/test_embedded_model.py Outdated Show resolved Hide resolved
tests/mongo_fields/test_listfield.py Outdated Show resolved Hide resolved
tests/mongo_fields/test_listfield.py Outdated Show resolved Hide resolved
@timgraham
Copy link
Collaborator Author

This is not at all ready for review.

@timgraham timgraham force-pushed the embedded-model-field branch from 6019a25 to df74d7b Compare November 4, 2024 23:19
@timgraham timgraham changed the title add support for ListField and EmbeddedModelField add support for EmbeddedModelField Nov 4, 2024
@timgraham timgraham force-pushed the embedded-model-field branch from df74d7b to a52ad67 Compare November 9, 2024 00:52
@timgraham timgraham force-pushed the embedded-model-field branch 3 times, most recently from 0db42ed to 09c1d69 Compare November 22, 2024 20:11
@timgraham timgraham force-pushed the embedded-model-field branch from 6f44d29 to da58861 Compare November 26, 2024 03:01
@timgraham timgraham force-pushed the embedded-model-field branch from e2947b8 to dd65bb7 Compare December 13, 2024 20:32
@timgraham timgraham force-pushed the embedded-model-field branch from dd65bb7 to 62cafa8 Compare December 30, 2024 18:57
@timgraham timgraham force-pushed the embedded-model-field branch from 62cafa8 to b61cf56 Compare January 6, 2025 22:27
@timgraham timgraham changed the title add support for EmbeddedModelField add EmbeddedModelField Jan 7, 2025
@timgraham timgraham force-pushed the embedded-model-field branch 5 times, most recently from 63a7fad to 5f5fcd4 Compare January 10, 2025 15:28
@timgraham timgraham force-pushed the embedded-model-field branch from 5f5fcd4 to aed181d Compare January 12, 2025 01:29
@timgraham timgraham marked this pull request as ready for review January 13, 2025 17:38
@timgraham timgraham requested a review from Jibola January 13, 2025 17:38
...
}

Querying ``EmbeddedModelField``
Copy link
Collaborator

@Jibola Jibola Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also specify users can query passing in an arbitrary model; however, we do not recommend querying this as results look for an exact match where ordering matters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This query could work with changes in progress for ArrayField + EMF (implementing EmbeddedModelField.get_db_prep_value()) but it currently crashes like bson.errors.InvalidDocument: cannot encode object: <Author: Author object (None)>, of type: <class 'model_fields_.models.Author'> since the model instance isn't transformed to dict. However, I'm very wary of this pattern as I think it will be a huge footgun (the queries work fine initially, then mysteriously break after a schema change... then what is the user to do?).

Comment on lines +51 to +53
You can query into an embedded model using the same double underscore syntax
as relational fields. For example, to retrieve all customers who have an
address with the city "New York"::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also emphasize that you can query using a dictionary, but we that's not

Customer.objects.filter(address={..."city": "New York"})

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it works. e.g. Book.objects.filter(author={"name": "Shakespeare"}) gives db.model_fields__book.aggregate([{'$match': {'$expr': {'$eq': ['$author', {'name': 'Shakespeare'}]}}}]). I think you're thinking of Djongo's query syntax.

@timgraham timgraham force-pushed the embedded-model-field branch from aed181d to 07525fe Compare January 14, 2025 00:21
@timgraham timgraham force-pushed the embedded-model-field branch from e522e32 to f77e03c Compare January 14, 2025 12:55
@timgraham timgraham merged commit f77e03c into mongodb-labs:main Jan 14, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PYTHON-4625: add support for EmbeddedModelField
5 participants